8356870: HotSpotDiagnosticMXBean.dumpThreads and jcmd Thread.dump_to_file updates #25429
Conversation
|
👋 Welcome back alanb! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@AlanBateman The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
|
/label remove hotspot |
|
@AlanBateman |
|
@AlanBateman |
|
@AlanBateman |
Webrevs
|
src/java.base/share/classes/jdk/internal/vm/ThreadSnapshot.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/vm/ThreadSnapshot.java
Outdated
Show resolved
Hide resolved
| * text or JSON format. | ||
| * This class defines static methods to support the Thread.dump_to_file diagnostic command | ||
| * and the HotSpotDiagnosticMXBean.dumpThreads API. It defines methods to generate a | ||
| * thread dump to a file or byte array in plain text or JSON format. | ||
| */ | ||
| public class ThreadDumper { |
There was a problem hiding this comment.
| public class ThreadDumper { | |
| public final class ThreadDumper { |
| sb.append(String.format("\\u%04x", c)); | ||
| } else { | ||
| sb.append(c); | ||
| private static class JsonWriter { |
There was a problem hiding this comment.
| private static class JsonWriter { | |
| private static final class JsonWriter { | |
| private static final class Node { |
test/jdk/com/sun/management/HotSpotDiagnosticMXBean/DumpThreadsWithEliminatedLock.java
Show resolved
Hide resolved
src/java.base/share/classes/jdk/internal/vm/ThreadSnapshot.java
Outdated
Show resolved
Hide resolved
|
Sync'ed up to the pull/25425. |
| for (StackTraceElement ste : thread.getStackTrace()) { | ||
| ps.print(" "); | ||
| ps.println(ste); | ||
| private static void dumpThread(Thread thread, TextWriter writer) { |
There was a problem hiding this comment.
On the non-json text format for locks: here we're creating a new comment-like style:
// parked on ..etc...
In the regular Thread.print we always used a "-" prefix, and always printed the frame, then the relevant locks, like:
at ThreadsMem$2.run(ThreadsMem.java:38)
- waiting to lock <0x0000000630817da0> (a java.lang.Object)
at java.lang.ref.ReferenceQueue.remove(java.base@25-internal/ReferenceQueue.java:215)
- locked <0x0000000630802350> (a java.lang.ref.ReferenceQueue$Lock)
Could we use the same? We have a lot of history reading the established style. 8-)
Can we match the old-style "waiting to lock" rather than "waiting on" ?
I realise I'm asking to move the printing of "waiting to lock" into the loop over the stackframes, and it affects various tests.
There was a problem hiding this comment.
When parked and there is a parkBlocker, blocked entering a monitor, or waiting in Object.wait, then it gets printed between the summary/state (first line) and the stack trace. I think this is a bit clearer that printing it after the top frame but okay to change. Note that the output isn't going to look the same as the traditional thread dump as it prints the object's identity hashcode rather than the address.
For the "locked" output then you may have a point, been back and forth on this. For synchronized methods then I think it's clearer when it is printed between the caller and synchronized callee. For synchronized blocks then it's clearer when to see which method has entered the monitor. Picking one means it's not clear in all cases but maybe people are just so used to "- locked" and don't notice. Can look at this again, it's trivial to swap between the two.
| jsonWriter.writeProperty("time", now); | ||
| if (thread.isVirtual()) { | ||
| jsonWriter.writeProperty("virtual", Boolean.TRUE); | ||
| } |
There was a problem hiding this comment.
Q: Is the order of dumped properties important?
The JSON schema lists the virtual property after the state:
51 "tid": {
52 "type": "string",
53 "description": "The thread identifier."
54 },
55 "time": {
56 "type": "string",
57 "description": "The time in ISO 8601 format that the thread was sampled."
58 },
59 "name": {
60 "type": "string",
61 "description": "The thread name."
62 },
63 "state": {
64 "type": "string",
65 "description": "The thread state (Thread::getState)."
66 },
67 "virtual" : {
68 "type": "boolean",
69 "description": "true for a virtual thread."
70 },
The other properties dumped in the dumpThread() follow the order defined in JSON schema.
There was a problem hiding this comment.
Q: Is the order of dumped properties important?
No, the ordering doesn't matter. The intent with the schema in the API docs is that we have somewhere to describe the objects name properties. If you re-order then it still validate.
Updates the thread dump generated by HotSpotDiagnosticMXBean.dumpThreads and jcmd Thread.dump_to_file to include thread state and lock information. Also update the HotSpotDiagnosticMXBean.dumpThreads API description to link to a description of the JSON format dump as that format is intended to be parseable/read by tools.
This PR is dependent on pull/25425. As noted in that PR, the changes accumulated in the loom repo, and have been split up to make it easier to review.
The changes include some re-implementation of ThreadDumper. This is because it used PrintStream and didn't fail if there was an I/O error, e.g. file system full. Furthermore, the indentation to pretty print the json was fragile and hard to maintain so this is changed to use a supporting writer class to do this.
Test coverage is significantly expanded, including updating the test library that is used by several tests to parse the thread dump.
Testing: tier1-6
Progress
Integration blocker
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25429/head:pull/25429$ git checkout pull/25429Update a local copy of the PR:
$ git checkout pull/25429$ git pull https://git.openjdk.org/jdk.git pull/25429/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25429View PR using the GUI difftool:
$ git pr show -t 25429Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25429.diff
Using Webrev
Link to Webrev Comment